-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove Breez SDK dependency #29
Conversation
978ed78
to
995e414
Compare
@@ -28,7 +25,7 @@ class CurrencyBloc extends Cubit<CurrencyState> with HydratedMixin { | |||
} | |||
|
|||
void listFiatCurrencies() { | |||
_breezSDK.listFiatCurrencies().then((fiatCurrencies) { | |||
liquidSdk.wallet!.listFiatCurrencies().then((fiatCurrencies) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this isn't under review (sorry for throwing the 🔧). I think name wallet
might become confusing for devs using the SDK, especially when listing fiat rates etc. They might assume its access to the underlying liquid wallet. Something like instance
might be better (if it's not a reserved word) liquidSdk.instance!.listFiatCurrencies()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Let's open an issue for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
inputType is InputType_LnUrlWithdraw || | ||
inputType is InputType_LnUrlAuth || | ||
inputType is InputType_LnUrlError || | ||
inputType is InputType_NodeId)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think NodeId would also not be supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted. It'll be removed as part of #30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the reviews. I'll merge after adding CI requirements. |
88ec2d1
to
22528b7
Compare
*Shared modules are moved to sdk-common and is now accessible from Liquid SDK. *Update CurrencyBloc to use FiatAPI
closes #22
- Remove setting up Breez SDK steps - Remove choosing a Breez SDK ref for workflow
22528b7
to
aafea00
Compare
Depends on:
FiatAPI
methods to Liquid SDK breez-sdk-liquid#331This PR removes Breez SDK as shared modules are fully moved to
sdk-common
and is now accessible from Liquid SDK.Changelist:
CurrencyBloc
to useFiatAPI
CurrencyConverterDialog